ratelimit: add support for x-ratelimit-* headers in local rate limiting by zhxie · Pull Request #18157 · envoyproxy/envoy

zhxie · 2021-09-17T05:49:37Z

Signed-off-by: Xie Zhihao zhihao.xie@intel.com

Commit Message: ratelimit: add support for x-ratelimit-* headers in local rate limiting
Additional Description:
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A
Fixes #15293
Deprecated: the envoy::extensions::filters::http::ratelimit::v3::RateLimit::XRateLimitHeadersRFCVersion is deprecated in favor of envoy::extensions::common::ratelimit::v3::XRateLimitHeadersRFCVersion

repokitteh-read-only · 2021-09-17T05:49:39Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18157 was opened by zhxie.

see: more, trace.

repokitteh-read-only · 2021-09-17T05:49:44Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18157 was opened by zhxie.

see: more, trace.

mathetake · 2021-09-22T06:10:47Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

looks like you always give init_fill_time = true. Do we need the param in the first place?

No, in local rate limit network filter, init_fill_time = false. A test of local rate limit network filter will throw an exception for different time system if we initialize fill_time_ in LocalRateLimiterImpl for local rate limit network filter.

Do we know why this is the case? What is the implication of not initializing the fill time at ctor time?

Even though this is used for calculating the default per-route RateLimit_Reset which is new header field for HTTP only, I am interested to learn why it fails in network filter? This code seems not to be something HTTP filter specific?

Ok, it is a mistake, and I suppose it is because I was not not familiar about the time system in the test before.

There is a assert failure threw by the test shows there is an inconsistent time system. And today I find the problem is only in the test code, not in the source code.

But I cannot fix the test. There is a conflict between the MockDispatcher which uses the GlobalTimeSystem and the MockStreamInfo in MockReadFilterCallbacks which uses the SimulatedTimeSystem. Do you have some ideas?

There is already a TODO(#4160) in class GlobalTimeSystem: Switch default testSystem to SimulatedTimeSystem. I guess switching to that system might resolve the issue? I don't have the context on that TODO though.

If that is a lot of work to do or if that doesn't work, a quick workaround off top of my head (just for ideas not necessary to be the final solution): wrapping this line under if condition like below:

if (!descriptors.empty()) { tokens_.fill_time_ = time_source_.monotonicTime(); }

This implicitly limits this code path to local descriptor override case which is only available in HTTP filter.
It is a bit hacky workaround :) but, IMO, it is still better than having bool flag in the interface all over the places.

Unfortunately, I do not think it might work. In fact, a HTTP filter should load time to fill_time_ while its descriptors are empty.

mathetake · 2021-09-22T06:15:36Z

source/extensions/filters/http/local_ratelimit/local_ratelimit.cc

I Imagine we don't need to have optional here. Do you consider just avoiding push(abls::nullopt)?

We still need an optional here. In any request the filter is not enabled, we will not attach x-ratelimit-* headers.

adisuissa

Thanks for working on this!
Left an API comment.

adisuissa · 2021-09-22T14:03:18Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

How is this enum different than:

envoy/api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

Lines 28 to 34 in 1aba2de

enum XRateLimitHeadersRFCVersion {

// X-RateLimit headers disabled.

OFF = 0;

// Use `draft RFC Version 03 <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.

DRAFT_VERSION_03 = 1;

}

?
I think we should use the other enum, and not redefine a new one as this can cause a divergence in the future (e.g., global RL supporting draft 03, while local RL supporting draft 04). We should make this consistent.

DRAFT_VERSION_03 here means the responding x-ratelimit-* headers are according to draft RFC version 03. I think we should keep its name. And do you think it is a good idea for us to extract XRateLimitHeadersRFCVersion in LocalRateLimit and RateLimit and relocate it in common/ratelimit/v3/ratelimit.proto?

I don't mind the name DRAFT_VERSION_03.

Yes, I do think it would be better to extract the enum to the extension's common ratelimit proto.

An API change has been committed, but the CI reports the change is non-backwards-compatible. Is this a issue that needs to be fixed?

adisuissa · 2021-09-22T14:09:59Z

generated_api_shadow/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

The generated_api_shadow directory was removed in #18091.
Please remove this file

Ok, recent commits have been merged.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

adisuissa

Left some API comments.

adisuissa · 2021-09-28T12:30:16Z

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

      "envoy.config.filter.http.rate_limit.v2.RateLimit";

-  // Defines the version of the standard to use for X-RateLimit headers.
-  enum XRateLimitHeadersRFCVersion {


This should be deprecated.

adisuissa · 2021-09-28T12:35:37Z

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

  //
  // Disabled by default.
-  XRateLimitHeadersRFCVersion enable_x_ratelimit_headers = 8
+  common.ratelimit.v3.XRateLimitHeadersRFCVersion enable_x_ratelimit_headers = 8


Unfortunately this breaks backwards compatibility, and therefore the field must be deprecated and a new field with the new type should be introduced.

@htuch extracting the enum to a common place (see: api/envoy/extensions/common/ratelimit/v3/ratelimit.proto) is arguably the right way to go, but will cause some deprecation churn. Another option is to make the local_rate_limit proto use the enum directly from the (global) rate_limit proto. WDYT?

I think for the local rate limiting, put it in the common location and use it there. Either deprecate or eave a TODO to cleanup for global. I'm not super keen on API dependencies between extensions, since we're creating an unnecessary dependency in build/protos.

In my opinion, if we treat local rate limit and rate limit as two independent extensions (like if we can decouple envoy and extensions someday in the future), both local rate limit and rate limit should maintain their own enum values. But if we consider the local rate limit and rate limit are inseparable, extract common enum is a good choice.

My take on this is that similar to the original issue (#15293) that requested a local rate-limiting feature support similar to what the global rate-limiting does, then in the future they should be in-sync.
I think that adding the enum to the common proto (as you've already done) is a good thing, and to avoid changes to the global rate-limiting API (following @htuch suggestion), we should not deprecate this enum at this stage, but add a [#next-major-version: comment (see: example here).

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie · 2021-09-30T01:28:53Z

/retest

repokitteh-read-only · 2021-09-30T01:28:57Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18157 (comment) was created by @zhxie.

see: more, trace.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

snowp

Some nits but otherwise this looks good to me

/wait

snowp · 2022-02-07T16:24:20Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+                         : requestAllowedHelper(tokens_);
+}
+
+uint32_t LocalRateLimiterImpl::maxTokens(


We tend to favor longer names if it helps readability, but it's probably ok as is

snowp · 2022-02-07T16:26:34Z

test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc

+                            *descriptors_.Add());
+  initializeWithDescriptor(std::chrono::milliseconds(3000), 2, 2);
+
+  /// 2 -> 1 tokens


nit: extra /

Thanks, I will fix it.

snowp · 2022-02-07T16:27:44Z

test/extensions/filters/http/local_ratelimit/filter_test.cc


 TEST_F(FilterTest, Runtime) {
-  setup(fmt::format(config_yaml, "1", "false"), false, false);
+  setup(fmt::format(config_yaml, "1", "false", "\"OFF\""), false, false);


I don't think you need to include the quotes in there? You don't when you specify the other option

No, we cannot remove these quotes. We write YAML like

local_rate_limit_per_downstream_connection: false enable_x_ratelimit_headers: "OFF"

, so we must keep these quotes.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

snowp

LGTM, thanks!

snowp · 2022-02-17T21:52:25Z

@adisuissa mind doing the API sign off?

adisuissa

/lgtm api

adisuissa · 2021-10-29T18:54:40Z

api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto

  // Defines the version of the standard to use for X-RateLimit headers.
+  // This field is deprecated in favor of :ref:`XRateLimitHeadersRFCVersion <envoy_v3_api_enum_extensions.common.ratelimit.v3.XRateLimitHeadersRFCVersion>`.
  enum XRateLimitHeadersRFCVersion {
+    option deprecated = true;


I'm on the fence whether this should be deprecated while enable_x_ratelimit_headers isn't deprecated and a new alternative isn't introduced (that is, both should either be deprecated or not).
I guess that for now you can remove the deprecation, and add #next-major-version to your comment above.

So, we are going to unify the XRateLimitHeadersRFCVersion used by rate limit and local rate limit. I will change the deprecated to a #next-major-version.

adisuissa · 2021-10-29T19:44:53Z

source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc

+    for (const auto& request_descriptor : request_descriptors) {
+      auto it = descriptors_.find(request_descriptor);
+      if (it != descriptors_.end()) {
+        return static_cast<uint32_t>(absl::ToInt64Seconds(


Is there some expected bound to the result of ToInt64Seconds(...)? Could it overflow or cause issues with the cast to uint32_t?

I have changed the return type from uint32_t to int64_t in previous commits, so we do not need the cast anymore.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie · 2022-02-28T02:42:33Z

/retest

repokitteh-read-only · 2022-02-28T02:42:37Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18157 (comment) was created by @zhxie.

see: more, trace.

rojkov · 2022-03-03T08:55:45Z

@adisuissa could you please reinstate the API approval?

adisuissa

/lgtm api

…ng (envoyproxy#18157) ratelimit: add support for x-ratelimit-* headers in local rate limiting Signed-off-by: Xie Zhihao <zhihao.xie@intel.com> Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

repokitteh-read-only bot added the api label Sep 17, 2021

repokitteh-read-only bot assigned adisuissa Sep 17, 2021

zhxie force-pushed the header-lrl branch 2 times, most recently from f532236 to 79e0f66 Compare September 18, 2021 04:58

zhxie marked this pull request as ready for review September 22, 2021 04:52

zhxie requested review from junr03 and mattklein123 as code owners September 22, 2021 04:52

mathetake reviewed Sep 22, 2021

View reviewed changes

alyssawilk assigned junr03 Sep 22, 2021

adisuissa requested changes Sep 22, 2021

View reviewed changes

adisuissa reviewed Sep 22, 2021

View reviewed changes

zhxie added 9 commits September 23, 2021 10:31

ratelimit: add support for x-ratelimit-* headers in local rate limiting

1940c6c

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

ratelimit: escape unabled requests from appending headers

5482d0d

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

ratelimit: fix crash on description-based rate limit

e5230d9

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test for local rate limiter

a71c1ca

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

ratelimit: escape from crash in test of network filter

b356032

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: fix build failure

36c31d6

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: fix format

98101be

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test for x-ratelimit-* headers

0fe03e8

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

ratelimit: remove generated_api_shadow

612e6f1

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie force-pushed the header-lrl branch from 17825fd to 612e6f1 Compare September 23, 2021 02:35

ratelimit: merge x-ratelimit-* headers enum

47c9f6d

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

adisuissa reviewed Sep 28, 2021

View reviewed changes

zhxie added 3 commits September 29, 2021 10:17

ratelimit: deprecate original x-ratelimit-* headers enum

92d18f2

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

Merge remote-tracking branch 'envoyproxy/main' into header-lrl

5f26554

ratelimit: fix docs

20907cc

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

nit: remove redundant

c601b82

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only bot removed the waiting label Jan 30, 2022

junr03 removed their assignment Feb 2, 2022

snowp suggested changes Feb 7, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 7, 2022

yanavlasov self-assigned this Feb 8, 2022

zhxie added 3 commits February 14, 2022 09:26

Merge remote-tracking branch 'envoyproxy/main' into header-lrl

6a57280

docs: update release notes

31c7daa

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

nit: remove extra comment slash

cdb1471

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only bot removed the waiting label Feb 14, 2022

docs: fix release notes

124f0ef

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

snowp previously approved these changes Feb 17, 2022

View reviewed changes

adisuissa reviewed Feb 18, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Feb 18, 2022

snowp added the waiting:any label Feb 18, 2022

repokitteh-read-only bot removed the waiting:any label Feb 21, 2022

api: do not deprecate XRateLimitHeadersRFCVersion

4b74159

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie dismissed snowp’s stale review via 4b74159 February 21, 2022 01:32

repokitteh-read-only bot added the api label Feb 21, 2022

adisuissa reviewed Mar 10, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Mar 10, 2022

adisuissa approved these changes Mar 10, 2022

View reviewed changes

adisuissa merged commit 5c56e45 into envoyproxy:main Mar 10, 2022

adisuissa mentioned this pull request Aug 31, 2022

local ratelimit support virtualhost config #22895

Merged

zhxie deleted the header-lrl branch September 7, 2022 08:28

	enum XRateLimitHeadersRFCVersion {
	// X-RateLimit headers disabled.
	OFF = 0;

	// Use `draft RFC Version 03 <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.
	DRAFT_VERSION_03 = 1;
	}

Conversation

zhxie commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 17, 2021

Uh oh!

repokitteh-read-only bot commented Sep 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhxie Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhxie commented Sep 30, 2021

Uh oh!

repokitteh-read-only bot commented Sep 30, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

zhxie commented Sep 17, 2021 •

edited

Loading

tyxia Jan 18, 2022 •

edited

Loading

tyxia Jan 19, 2022 •

edited

Loading

zhxie Jan 20, 2022 •

edited

Loading

zhxie Feb 21, 2022 •

edited

Loading